-
Notifications
You must be signed in to change notification settings - Fork 10
Add API key support for the worker-controller #149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Supporting review comments in temporalio#149 which I'd like to see over the line.
@@ -0,0 +1,66 @@ | |||
--- | |||
apiVersion: rbac.authorization.k8s.io/v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was accidentally committed (sidenote, we should fix the make manifests
target to put these files in the right place after generating them, but for this PR just remove the extra files)
@@ -0,0 +1,26 @@ | |||
--- | |||
apiVersion: admissionregistration.k8s.io/v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
func resolveAuthSecretName(tc *temporaliov1alpha1.TemporalConnection) (clientpool.AuthMode, string, error) { | ||
auth := getAuthMode(tc) | ||
switch auth { | ||
case clientpool.AuthModeTLS: | ||
name, err := getTLSSecretName(tc.Spec.MutualTLSSecretRef) | ||
return auth, name, err | ||
case clientpool.AuthModeAPIKey: | ||
name, err := getAPIKeySecretName(tc.Spec.APIKeySecretRef) | ||
return auth, name, err | ||
default: | ||
return auth, "", nil | ||
} | ||
} | ||
|
||
func getAuthMode(temporalConnection *temporaliov1alpha1.TemporalConnection) clientpool.AuthMode { | ||
if temporalConnection.Spec.MutualTLSSecretRef != nil { | ||
return clientpool.AuthModeTLS | ||
} else if temporalConnection.Spec.APIKeySecretRef != nil { | ||
return clientpool.AuthModeAPIKey | ||
} | ||
return clientpool.AuthModeNoCredentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I only see getAuthMode
used in this one place, so it's more concise to just do:
func resolveAuthSecretName(tc *temporaliov1alpha1.TemporalConnection) (clientpool.AuthMode, string, error) {
if temporalConnection.Spec.MutualTLSSecretRef != nil {
name, err := getTLSSecretName(tc.Spec.MutualTLSSecretRef)
return clientpool.AuthModeTLS, name, err
} else if temporalConnection.Spec.APIKeySecretRef != nil {
name, err := getAPIKeySecretName(tc.Spec.APIKeySecretRef)
return clientpool.AuthModeAPIKey, name, err
}
return clientpool.AuthModeNoCredentials, "", nil
}
namespace: "" # e.g. default | ||
# Use existing connection (leave empty to create new one) | ||
connectionName: "" # e.g. dev-server | ||
# Connection details (required if connectionName is empty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Connection details (required if connectionName is empty) | |
# Connection details (either mtlsSecretName or apiKey is required if connectionName is empty) |
(Or feel free to say that only one or the other is required in another way)
"properties": { | ||
"name": { | ||
"type": "string", | ||
"description": "Name of the secret containing API key (required if connectionName is empty)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this says that both apiKey and mtlsSecretName are required but only one or the other is required -- right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved once the extra config/
files are deleted
What was changed
Why?
Checklist
Closes
Add API Keys support #144
How was this tested: